Skip to content

chore: upgrade ng from 11 to 12 #302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 17, 2021
Merged

chore: upgrade ng from 11 to 12 #302

merged 2 commits into from
Aug 17, 2021

Conversation

itssharmasandeep
Copy link
Contributor

Due to some problem with #297
This is the new one with addressed comments

@itssharmasandeep itssharmasandeep requested a review from a team as a code owner August 16, 2021 16:13
@github-actions

This comment has been minimized.

@codecov
Copy link

codecov bot commented Aug 16, 2021

Codecov Report

Merging #302 (213334f) into main (edb0bbb) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #302   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           49        49           
  Lines          655       655           
  Branches        62        62           
=========================================
  Hits           655       655           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edb0bbb...213334f. Read the comment docs.

"@angular/forms": "^12.2.1",
"@angular/platform-browser": "^12.2.1",
"@angular/platform-browser-dynamic": "^12.2.1",
"@angular/router": "^12.2.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about zone.js? does it need to be upgraded?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think we're already on at latest one https://www.npmjs.com/package/zone.js?activeTab=versions

@@ -14,4 +14,4 @@ export const environment = {
* This import should be commented out in production mode because it will have a negative impact
* on performance if an error is thrown.
*/
// import 'zone.js/dist/zone-error'; // Included with Angular CLI.
// import 'zone.js/plugins/zone-error'; // Included with Angular CLI.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this change here but no change in zone.js dependency. Is this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, answered above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

above you answered that zone.js doesn't need to be upgraded - but I think anand is asking: then why would this change? was it previously wrong?

It looks like the zone docs agree that it was:
https://github.com/angular/angular/tree/master/packages/zone.js#bundles

@anandtiwary
Copy link
Contributor

@itssharmasandeep quick question: What is peer dependency in npm?

@@ -14,4 +14,4 @@ export const environment = {
* This import should be commented out in production mode because it will have a negative impact
* on performance if an error is thrown.
*/
// import 'zone.js/dist/zone-error'; // Included with Angular CLI.
// import 'zone.js/plugins/zone-error'; // Included with Angular CLI.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

above you answered that zone.js doesn't need to be upgraded - but I think anand is asking: then why would this change? was it previously wrong?

It looks like the zone docs agree that it was:
https://github.com/angular/angular/tree/master/packages/zone.js#bundles

@anandtiwary
Copy link
Contributor

We need to upgrade peer dependency before merging this pr. i have shared reading material with sandeep on slack

Co-authored-by: Aaron Steinfeld <[email protected]>
@github-actions

This comment has been minimized.

@aaron-steinfeld
Copy link
Contributor

We need to upgrade peer dependency before merging this pr. i have shared reading material with sandeep on slack

right - forgot this repo even had the separate project

@itssharmasandeep itssharmasandeep merged commit 84b0544 into main Aug 17, 2021
@itssharmasandeep itssharmasandeep deleted the angular-upgrade branch August 17, 2021 16:30
@itssharmasandeep
Copy link
Contributor Author

taking peerDependency part separately

@github-actions
Copy link

Unit Test Results

  2 files  15 suites   24s ⏱️
53 tests 53 ✔️ 0 💤 0 ❌

Results for commit 84b0544.

@aaron-steinfeld
Copy link
Contributor

taking peerDependency part separately

why? without the peer dependencies this isn't usable (so shouldn't have been merged). what am I missing?

@anandtiwary
Copy link
Contributor

anandtiwary commented Aug 17, 2021

@aaron-steinfeld I want sandeep to try out and learn the entire process. Explained him few things, but a good hands on would help him learn the entire flow. So, I asked him to merge this PR and see what is breaking in ht-ui when he tries to add the new bundle.

@github-actions
Copy link

🎉 This PR is included in version 2.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants